Remove fewer Storage calls in CopyProp and GVN #142531
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
bors
added a commit
that referenced
this pull request
Jun 15, 2025
…try>
Remove fewer Storage calls in `copy_prop`
Modify the `copy_prop` MIR optimization pass to remove fewer `Storage{Live,Dead}` calls, allowing for better optimizations by LLVM - see #141649.
### Details
This is my attempt to fix the mentioned issue (this is the first part, I also implemented a similar solution for GVN in [this branch](https://github.com/rust-lang/rust/compare/master...ohadravid:rust:better-storage-calls-gvn-v2?expand=1)).
The idea is to use the `MaybeStorageDead` analysis and remove only the storage calls of `head`s that are maybe-storage-dead when the associated `local` is accessed (or, conversely, keep the storage of `head`s that are for-sure alive in _every_ relevant access).
When combined with the GVN change, the final example in the issue (#141649 (comment)) is optimized as expected by LLVM. I also measured the effect on a few functions in `rav1d` (where I originally saw the issue) and observed reduced stack usage in several of them.
This is my first attempt at working with MIR optimizations, so it's possible this isn't the right approach — but all tests pass, and the resulting diffs appear correct.
r? tmiasko
since he commented on the issue and pointed to these passes.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (ef7d206): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.7%, secondary 3.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.6%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 757.399s -> 756.065s (-0.18%) |
rustbot
added
perf-regression
|
@matthiaskrgr - I updated the impl to stop re-checking once a head is found to be maybe-dead, which should be a bit better |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
bors
added a commit
that referenced
this pull request
Jun 15, 2025
…try>
Remove fewer Storage calls in `copy_prop`
Modify the `copy_prop` MIR optimization pass to remove fewer `Storage{Live,Dead}` calls, allowing for better optimizations by LLVM - see #141649.
### Details
This is my attempt to fix the mentioned issue (this is the first part, I also implemented a similar solution for GVN in [this branch](https://github.com/rust-lang/rust/compare/master...ohadravid:rust:better-storage-calls-gvn-v2?expand=1)).
The idea is to use the `MaybeStorageDead` analysis and remove only the storage calls of `head`s that are maybe-storage-dead when the associated `local` is accessed (or, conversely, keep the storage of `head`s that are for-sure alive in _every_ relevant access).
When combined with the GVN change, the final example in the issue (#141649 (comment)) is optimized as expected by LLVM. I also measured the effect on a few functions in `rav1d` (where I originally saw the issue) and observed reduced stack usage in several of them.
This is my first attempt at working with MIR optimizations, so it's possible this isn't the right approach — but all tests pass, and the resulting diffs appear correct.
r? tmiasko
since he commented on the issue and pointed to these passes.
|
Should this check happen in |
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
I'm not sure how to make this work: using Is there a different way to do this? |
|
Finished benchmarking commit (c0a2949): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.1%, secondary -1.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 756.494s -> 757.685s (0.16%) |
rustbot
removed
the
S-waiting-on-perf
tmiasko
reviewed
Jun 15, 2025
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@ohadravid do you mind merging this PR and #142819? Both should use the same code to decide whether to keep or remove storage statements. And I fear that having 2 PRs mean that @tmiasko and I won't see each other ideas and give you diverging advice. |
ohadravid
force-pushed
the
better-storage-calls-copy-prop
branch
from
fdcc8a6 to
26fc160
Compare
|
@cjgillot , @tmiasko - merged both PR here. Current impls are based on the new Does GVN require an additional check against borrowed locals like mentioned in #142531 (comment)? Both only do the more complex analysis when And thank you both for reviewing these and explaining everything! 🙏 |
ohadravid
changed the title
copy_prop
tmiasko
reviewed
Jun 25, 2025
tests/mir-opt/copy-prop/copy_prop_borrowed_storage_not_removed.rs
Outdated
Show resolved
Hide resolved
|
I am not familiar with GVN, so I will leave review of that part to @cjgillot . |
|
☔ The latest upstream changes (presumably #142870) made this pull request unmergeable. Please resolve the merge conflicts. |
… to remove fewer storage statements
…r storage statements
ohadravid
force-pushed
the
better-storage-calls-copy-prop
branch
from
26fc160 to
5c21ce3
Compare
This comment has been minimized.
This comment has been minimized.
ohadravid
force-pushed
the
better-storage-calls-copy-prop
branch
from
654c4a2 to
ab1da60
Compare
This comment has been minimized.
This comment has been minimized.
ohadravid
force-pushed
the
better-storage-calls-copy-prop
branch
from
ab1da60 to
e69bef8
Compare
This comment has been minimized.
This comment has been minimized.
|
@tmiasko - implemented all the changes 😄 I also updated the GVN code since they applied there are well (use a single I can also split this PR if needed, and I'll polish the git history when you think this looks good enough 🧹 PS |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Modify the CopyProp and GVN MIR optimization passes to remove fewer
Storage{Live,Dead}calls, allowing for better optimizations by LLVM - see #141649.Details
The idea is to use a new
MaybeUninitializedLocalsanalysis and remove only the storage calls of locals that are maybe-uninit when accessed in a new location.